chore: Clean orphan charts before generating #17584
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning [email protected]: This version is no longer supported. Please see https://eslint.org/version-support for other options. WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
infra/src/cli/generate-chart-values.ts (3)
24-24: Consider making the chart directory path more robust.While the relative path works, consider:
- Using
path.resolveto get an absolute path- Adding validation to ensure the directory exists before operations
-const relativeChartDir = '../../../charts' +const chartDir = path.resolve(__dirname, '../../../charts') +if (!fs.existsSync(chartDir)) { + throw new Error(`Charts directory not found: ${chartDir}`) +}
25-34: Enhance error handling and add success logging.The error handling could be more specific and include success logging:
const cleanOrphanDirs = (filePath: string): void => { try { + if (!fs.existsSync(filePath)) { + console.log(`No orphan charts to clean at ${filePath}`) + return + } rmSync(filePath, { recursive: true }); + console.log(`Successfully cleaned orphan charts at ${filePath}`) } catch (error) { - if (error instanceof Error) { - console.error(`Failed to clean orphan directories at ${filePath}: ${error.message}`); - } + if (error instanceof Error && error.code === 'ENOENT') { + console.log(`Directory ${filePath} does not exist`) + return + } + console.error(`Failed to clean orphan directories at ${filePath}:`, + error instanceof Error ? error.message : 'Unknown error'); } }
60-60: Maintain consistency in path construction.There's an inconsistency in path construction methods:
- Line 60 uses
path.join- Line 82 uses string interpolation
- `${relativeChartDir}/${name}-services`, + path.join(relativeChartDir, `${name}-services`),Also applies to: 82-82
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infra/src/cli/generate-chart-values.ts(5 hunks)
🔇 Additional comments (3)
infra/src/cli/generate-chart-values.ts (3)
3-3: LGTM! Appropriate use ofrmSyncfor directory removal.The addition of
rmSyncto the existing fs imports is well-placed and suitable for recursive directory cleanup.
45-46: LGTM! Well-placed cleanup step.The cleanup step is appropriately placed at the beginning of the chart generation process, with clear logging.
45-46: Verify the impact of chart cleanup.Let's ensure the cleanup doesn't affect active charts.
✅ Verification successful
Chart cleanup implementation is safe ✅
The cleanup operation only affects the generated charts in the
charts/directory and does not impact the actual Helm charts stored ininfra/helm/. The directory is properly recreated with fresh content after cleanup.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential active charts that might be affected fd -t d . charts/ -x sh -c ' if [ -f "$0/Chart.yaml" ]; then echo "Active chart found: $0" cat "$0/Chart.yaml" | grep -E "name:|version:" fi 'Length of output: 156
Script:
#!/bin/bash # Find the actual chart directory structure fd -t d "chart" . # Check the cleanOrphanDirs implementation ast-grep --pattern 'function cleanOrphanDirs($_) { $$$ }' # Look for Chart.yaml files anywhere in the repo fd "Chart.yaml"Length of output: 815
Script:
#!/bin/bash # Find the cleanOrphanDirs implementation rg "cleanOrphanDirs" -A 5 # Check the relative path configuration rg "relativeChartDir" -A 2Length of output: 1997
Datadog ReportAll test runs ✅ 10 Total Test Services: 0 Failed, 10 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (2) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
infra/src/cli/generate-chart-values.ts (2)
24-24: Consider using path.join for cross-platform compatibilityThe relative path string might be brittle across different platforms. Consider using
path.join()to construct the path.-const relativeChartDir = '../../../charts' +const relativeChartDir = path.join('..', '..', '..', 'charts')
83-83: Use consistent path joining methodString interpolation is used for path construction instead of
path.join()as used elsewhere in the code.- `${relativeChartDir}/${name}-services`, + path.join(relativeChartDir, `${name}-services`),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infra/src/cli/generate-chart-values.ts(5 hunks)
🔇 Additional comments (2)
infra/src/cli/generate-chart-values.ts (2)
3-3: LGTM: Import statement correctly updatedThe addition of
rmSyncto the fs import is appropriate for the new cleanup functionality.
46-47: Verify cleanup success before proceedingThe cleanup process should be validated before continuing with chart generation.
Consider adding a try-catch block around the cleanup and chart generation:
- console.log('Cleaning orphan charts') - cleanOrphanDirs(path.join(__dirname, relativeChartDir)) - console.log('Gathering charts') + try { + console.log('Cleaning orphan charts') + cleanOrphanDirs(path.join(__dirname, relativeChartDir)) + console.log('Gathering charts') + } catch (error) { + console.error('Failed to clean orphan charts, aborting generation') + throw error + }
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17584 +/- ##
==========================================
- Coverage 35.57% 35.57% -0.01%
==========================================
Files 7027 7027
Lines 150434 150434
Branches 42943 42943
==========================================
- Hits 53522 53519 -3
- Misses 96912 96915 +3
Flags with carried forward coverage won't be shown. Click here to find out more. see 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
* chore: Clean orphan charts before generating * chore: nx format:write update dirty files --------- Co-authored-by: andes-it <[email protected]>
...
Attach a link to issue if relevant
What
Specify what you're trying to achieve
Why
Specify why you need to achieve this
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
Chores
The changes optimize the chart generation workflow by automatically cleaning up orphan directories and centralizing path definitions.